Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hasfield to check for field existence #27582

Closed
wants to merge 3 commits into from

Conversation

iamed2
Copy link
Contributor

@iamed2 iamed2 commented Jun 14, 2018

I wanted this in another package but I figure it's general and useful enough for Base.

ararslan
ararslan previously approved these changes Jun 14, 2018
@ararslan ararslan added needs tests Unit tests are required for this change needs news A NEWS entry is required for this change labels Jun 14, 2018
@ararslan ararslan dismissed their stale review June 14, 2018 23:11

Needs tests and news

@iamed2
Copy link
Contributor Author

iamed2 commented Jun 15, 2018

The doctest is not enough? fieldindex itself only has doctests.

@ararslan
Copy link
Member

No, doctests are nice to have but pretty much everything should have a unit test. Weird that fieldindex doesn't have tests.

@JeffBezanson
Copy link
Sponsor Member

fieldindex probably didn't have tests since it's not exported.

I think this is fine for now. For future reference, I notice that both this and some existing functions like fieldoffset and fieldname should be expanded to handle more types using the technique that fieldcount uses.

@ararslan
Copy link
Member

Could you add an entry for this in NEWS.md as well?

@ararslan ararslan removed the needs tests Unit tests are required for this change label Jun 15, 2018
@ararslan ararslan removed the needs news A NEWS entry is required for this change label Jun 15, 2018
@iamed2
Copy link
Contributor Author

iamed2 commented Dec 26, 2018

Closing in favour of #30496

@iamed2 iamed2 closed this Dec 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants